8302673: [SuperWord] MaxReduction and MinReduction should vectorize for int #13924

robcasloz · 2023-05-11T09:18:21Z

The canonicalization of MinI/MaxI chains into right-spline graphs within MinINode/MaxINode::Ideal() inhibits the vectorization of reductions of these nodes. This changeset reworks MinINode/MaxINode::Ideal() to perform the same algebraic optimizations without the need for canonicalization, re-enabling auto-vectorization of MinI/MaxI reductions. This is achieved by handling all four permutations of the targeted Ideal subgraph induced by the commutativity of MinI/MaxI directly. The algorithm (for the MaxI case, the MinI case is analogous) tries to apply two Ideal graph rewrites in the following order, where c0 and c1 are constants and MAX is a compile-time operation:

max(x + c0, max(x + c1, z)) (or a permutation of it) to max(x + MAX(c0, c1), z).
max(x + c0, x + c1) (or a permutation of it) to x + MAX(c0, c1).

Here is an example of the four permutations handled in step 1 with x = RShiftI, c0 = 100 or 150, c1 = 150 or 100, and z = ConI (#int:200):

Here is an example of the two permutations handled in step 2 with x = RShiftI, c0 = 10 or 11, and c1 = 11 or 10:

The changeset implements MinINode/MaxINode::Ideal() in a common method MaxNode::IdealI(), since the algorithm is symmetric for both node types. The changeset also extends the existing MinI/MaxI Idealization tests with positive tests for all targeted permutations and negative tests, and adds a new test (contributed by @jbhateja) to assert that MinI/MaxI reductions are vectorized.

Testing

Functionality

tier1-5, stress test, fuzzing (windows-x64, linux-x64, linux-aarch64, macosx-x64, macosx-aarch64; release and debug mode).

Performance

Tested performance on a set of standard benchmark suites (DaCapo, SPECjbb2015, SPECjvm2008). No significant change was observed.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8302673: [SuperWord] MaxReduction and MinReduction should vectorize for int

Reviewers

Emanuel Peter (@eme64 - Committer)
Vladimir Kozlov (@vnkozlov - Reviewer)

Contributors

Jatin Bhateja <jbhateja@openjdk.org>

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/13924/head:pull/13924
$ git checkout pull/13924

Update a local copy of the PR:
$ git checkout pull/13924
$ git pull https://git.openjdk.org/jdk.git pull/13924/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 13924

View PR using the GUI difftool:
$ git pr show -t 13924

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/13924.diff

Webrev

Link to Webrev Comment

bridgekeeper · 2023-05-11T09:19:11Z

👋 Welcome back rcastanedalo! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2023-05-11T09:21:41Z

@robcasloz The following label will be automatically applied to this pull request:

hotspot-compiler

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

robcasloz · 2023-05-11T09:25:18Z

/contributor add @jbhateja

openjdk · 2023-05-11T09:26:35Z

@robcasloz
Contributor Jatin Bhateja <jbhateja@openjdk.org> successfully added.

mlbridge · 2023-05-12T07:58:46Z

Webrevs

test/hotspot/jtreg/compiler/loopopts/superword/MinMaxRed_Int.java

src/hotspot/share/opto/addnode.cpp

eme64 · 2023-05-17T14:30:39Z

Nice work with the tests, it's good to have some specific IR tests there!

I hope we can also generalize this for MaxL/MinL (once we do this JDK-8307513) - I think that is now also going to be easier with your refactoring towards MaxNode::IdealI.

eme64 · 2023-05-31T10:55:07Z

src/hotspot/share/opto/addnode.cpp

+        return new MinINode(add_transformed, inner_other);
+      } else {
+        return new MaxINode(add_transformed, inner_other);
+      }


Could you make use of MaxNode::build_min_max?

Done (extracting common functionality into build_min_max_int()), thanks!

eme64 · 2023-05-31T11:06:26Z

src/hotspot/share/opto/addnode.cpp

+static ConstAddOperands as_add_with_constant(Node* n) {
+  ConstAddOperands null(nullptr, 0);
+  if (n->Opcode() != Op_AddI) {
+    return null;
+  }
+  Node* x = n->in(1);
+  Node* c = n->in(2);
+  if (!c->is_Con()) {
+    return null;
+  }
+  const Type* c_type = c->bottom_type();
+  if (c_type == Type::TOP) {
+    return null;
+  }
+  return ConstAddOperands(x, c_type->is_int()->get_con());
+}


This is what it was on my last review:

// Return: // <x, C>, if n is of the form x + C, where 'C' is a non-TOP constant; // <nullptr, _>, if n is of the form x + C, where 'C' is a TOP constant; // <n, con> otherwise. static Node* constant_add_input(Node* n, jint* con) { if (n->Opcode() == Op_AddI && n->in(2)->is_Con()) { const Type* t = n->in(2)->bottom_type(); if (t == Type::TOP) { return nullptr; } *con = t->is_int()->get_con(); n = n->in(1); } return n; }

Here, you used to also allow packing just a single n, and leave the constant as zero. Did you remove this possibility on purpose? Now n must be an AddI.

This used to allow cases like this to be folded:
max(max(a, b), a + 1) -> max(a + max(0, 1), b)

Or am I missing something? Do you have tests for this case?

Good catch, thanks Emanuel! The simplification of as_add_with_constant() was too aggressive, and the lost optimizations were not caught by any test or noticeable regression on the standard Java benchmark suites. I added more test cases to catch them now and reverted as_add_with_constant() to return <n, 0> for non-additions.

eme64 · 2023-05-31T11:07:42Z

@robcasloz it looks much better, thanks for refactoring :)
I have left a few more comments.

…of individual components

…t' to its previous state

robcasloz · 2023-06-01T09:28:15Z

@eme64 Thanks for your thorough review, I addressed your new comments and suggestions. Please let me know if there is anything else you would like to change.

eme64 · 2023-06-01T10:29:30Z

src/hotspot/share/opto/addnode.cpp

+      }
+      Node* add_transformed = phase->transform(add_extracted);
+      Node* inner_other = inner_op->in(inner_add_index == 1 ? 2 : 1);
+      return build_min_max_int(add_transformed, inner_other, opcode == Op_MaxI);


Did something prevent you from directly using MaxNode::build_min_max?

Technically I think that should be possible, but I find that extracting the core logic into a separate function is more readable (makes it straightforward to understand the effect of the call in MaxNode::IdealI()) and efficient (avoids a redundant application of PhaseGVN::transform() to the newly created MinI/MaxI nodes).

eme64

Now it looks much cleaner, thanks for the new changes!
Thanks for the extra tests.

vnkozlov

Looks good to me too.

openjdk · 2023-06-01T16:02:56Z

@robcasloz This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8302673: [SuperWord] MaxReduction and MinReduction should vectorize for int

Co-authored-by: Jatin Bhateja <jbhateja@openjdk.org>
Reviewed-by: epeter, kvn

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 17 new commits pushed to the master branch:

6edd786: 8309265: Serial: Remove the code related to GC overheap limit
61bb014: 8309254: Implement fast-path for ASCII-compatible CharsetEncoders on RISC-V
62c935d: 8308978: regression with a deadlock involving FollowReferences
aff9cea: 8303530: Redefine JAXP Configuration File
1bb037b: 8309329: com/sun/jdi/DeferredStepTest.java fails with virtual threads due to not waiting for threads to exit
a23bbea: 8309241: ClassForNameLeak fails intermittently as the class loader hasn't been unloaded
931913f: 8309200: java/net/httpclient/ExecutorShutdown fails intermittently, if connection closed during upgrade
dc21e8a: 8296411: AArch64: Accelerated Poly1305 intrinsics
59d9d9f: 8303215: Make thread stacks not use huge pages
cb1e5e3: 8309286: G1: Remove unused G1HeapRegionAttr::is_valid_gen
... and 7 more: https://git.openjdk.org/jdk/compare/7b0a33600e27507546d38c53bdbc482561e1154b...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

robcasloz · 2023-06-02T07:07:36Z

Thanks for reviewing, Emanuel and Vladimir! I noticed that commit 29922ea in this PR accidentally fixes a latent issue in MaxNode::build_min_max(). I have reported and fixed the issue separately in JDK-8309295. Will rebase this PR and submit a new version after re-testing.

robcasloz · 2023-06-02T12:34:30Z

The latest version (v4) merges the fix from JDK-8309295 and prevents unnecessary idealization of MinI/MaxI nodes with transitive TOP constant inputs, which matches the original logic more closely. @eme64 @vnkozlov please re-review.

vnkozlov

Update is good.

eme64

Looks good, thanks for all the refactoring!

robcasloz · 2023-06-05T07:07:46Z

Emanuel, Vladimir, thanks again for reviewing, and particularly thanks Emanuel for the useful feedback!

robcasloz · 2023-06-05T07:07:53Z

/integrate

openjdk · 2023-06-05T07:08:43Z

Going to push as commit 3fa776d.
Since your change was applied there have been 24 commits pushed to the master branch:

22a9a86: 8309268: C2: "assert(in_bb(n)) failed: must be" after JDK-8306302
b6c9232: 8305225: A service broken error despite annotation processor generating it if directives listed
05fb6c6: 8309336: Incorrect switch in enum not reported properly
08c91c2: 8309332: RISC-V: Improve PrintOptoAssembly output of vector nodes
ecb1753: 8309334: ProcessTools.main() does not properly set thread names when using the virtual thread wrapper
ac1597b: 8309409: Update HttpInputStreamTest and BodyProcessorInputStreamTest to use hg.openjdk.org
fdb5893: 8309391: Remove non-failing tests from test/jdk/ProblemList-Virtual.txt
6edd786: 8309265: Serial: Remove the code related to GC overheap limit
61bb014: 8309254: Implement fast-path for ASCII-compatible CharsetEncoders on RISC-V
62c935d: 8308978: regression with a deadlock involving FollowReferences
... and 14 more: https://git.openjdk.org/jdk/compare/7b0a33600e27507546d38c53bdbc482561e1154b...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-06-05T07:08:58Z

@robcasloz Pushed as commit 3fa776d.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

robcasloz added 14 commits May 11, 2023 08:39

Clean up MinI/MaxI Ideal function

be1888f

Add basic test from openjdk#13260

9b218df

Added more Max test cases with swapped inputs

cd84227

Handle all four max-of-max cases without canonicalization

3973102

Extract MaxINode::Ideal() and MinINode::Ideal() into MaxNode::IdealI()

c56efa2

Flatten nested if

2149d34

Re-add comment

7adf075

Simplify

872bcbe

Refactor

d7c7987

Add some comments

89cebb1

Complement idealization tests with negative ones

d53cb53

Uncomment tests

3c12095

Update copyright header

83e7630

Refine comments

9fd482b

openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label May 11, 2023

robcasloz marked this pull request as ready for review May 12, 2023 07:52

openjdk bot added the rfr Pull request is ready for review label May 12, 2023

eme64 reviewed May 17, 2023

View reviewed changes

test/hotspot/jtreg/compiler/loopopts/superword/MinMaxRed_Int.java Outdated Show resolved Hide resolved

eme64 reviewed May 17, 2023

View reviewed changes

src/hotspot/share/opto/addnode.cpp Outdated Show resolved Hide resolved

eme64 reviewed May 17, 2023

View reviewed changes

src/hotspot/share/opto/addnode.cpp Outdated Show resolved Hide resolved

robcasloz added 4 commits May 22, 2023 17:30

Merge branch 'master' into JDK-8302673

80b100e

Merge branch 'master' into JDK-8302673

6308044

Randomize array values in min/max test computation

c736c0a

Make auxiliary add operand extraction function return a tuple

8f8faf2

eme64 reviewed May 31, 2023

View reviewed changes

robcasloz added 4 commits May 31, 2023 18:39

Extract MinI/MaxI construction; pass around ConstAddOperands instead …

29922ea

…of individual components

Add tests to exercise the case without inner additions

3adee22

Handle case without inner additions by restoring 'as_add_with_constan…

efb8ac0

…t' to its previous state

Complete test battery with remaining no-add cases

3b4d799

eme64 reviewed Jun 1, 2023

View reviewed changes

eme64 approved these changes Jun 1, 2023

View reviewed changes

robcasloz added 2 commits June 1, 2023 14:46

Revert extraction of min/max building

c26f585

Abort idealization if any of the adds has a TOP input

5a96171

vnkozlov approved these changes Jun 1, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jun 1, 2023

robcasloz mentioned this pull request Jun 1, 2023

8309295: C2: MaxNode::signed_min() returns nullptr for int operands #14272

Closed

3 tasks

Merge branch 'master' into JDK-8302673

f4837ac

Re-apply extraction of min/max building after JDK-8309295

cfcc16f

vnkozlov approved these changes Jun 2, 2023

View reviewed changes

eme64 approved these changes Jun 3, 2023

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Jun 5, 2023

openjdk bot closed this Jun 5, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jun 5, 2023

8302673: [SuperWord] MaxReduction and MinReduction should vectorize for int #13924

8302673: [SuperWord] MaxReduction and MinReduction should vectorize for int #13924

Uh oh!

Conversation

robcasloz commented May 11, 2023 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Testing

Functionality

Performance

Progress

Issue

Reviewers

Contributors

Reviewing

Webrev

Uh oh!

bridgekeeper bot commented May 11, 2023

Uh oh!

openjdk bot commented May 11, 2023

Uh oh!

robcasloz commented May 11, 2023

Uh oh!

openjdk bot commented May 11, 2023

Uh oh!

mlbridge bot commented May 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

Uh oh!

Uh oh!

Uh oh!

eme64 commented May 17, 2023

Uh oh!

eme64 May 31, 2023

Choose a reason for hiding this comment

Uh oh!

robcasloz Jun 1, 2023

Choose a reason for hiding this comment

Uh oh!

eme64 May 31, 2023

Choose a reason for hiding this comment

Uh oh!

robcasloz Jun 1, 2023

Choose a reason for hiding this comment

Uh oh!

eme64 commented May 31, 2023

Uh oh!

robcasloz commented Jun 1, 2023

Uh oh!

eme64 Jun 1, 2023

Choose a reason for hiding this comment

Uh oh!

robcasloz Jun 2, 2023

Choose a reason for hiding this comment

Uh oh!

eme64 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vnkozlov left a comment

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented Jun 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

robcasloz commented Jun 2, 2023

Uh oh!

robcasloz commented Jun 2, 2023

Uh oh!

vnkozlov left a comment

Choose a reason for hiding this comment

Uh oh!

eme64 left a comment

Choose a reason for hiding this comment

Uh oh!

robcasloz commented Jun 5, 2023

Uh oh!

robcasloz commented Jun 5, 2023

Uh oh!

openjdk bot commented Jun 5, 2023

robcasloz commented May 11, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented May 12, 2023 •

edited

Loading

eme64 left a comment •

edited

Loading

openjdk bot commented Jun 1, 2023 •

edited

Loading